-
Notifications
You must be signed in to change notification settings - Fork 162
First attempt at getting origami into Oscar #5345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This is certainly necessary to make the code calling it type stable
Again necessary to make code using it type stable.
... not as a Vector: a tuple can be allocated on the stack instead of the heap and thus avoid expensive allocations, and in general allows for more optimizations.
Otherwise, cperm creates a fresh parent group each time it is called...
There is (currently, at least) no need to have it mutable, and immutable potentially enables further operations, plus it may catch programming mistakes
- get rid of `degree_list` - make `normal_form` type stable by not reusing `G` as reference to objects of different types - avoid overhead in `cperm` for creating parent groups by creating a single shared parent group at the start. - instead of creating a vector of vector describing permutations; then turning that into a vector of permutations, directly store a vector of permutations (thus reducing the needed allocations) - replace `i^-1 * x * i` by `x^i` which is faster and again avoids allocations (at least when `x` and `i` are permutations, like here)
Improve `staircase_origami` and `normal_form`
| @@ -0,0 +1,33 @@ | |||
| function homology(o::Origami) | |||
| return GAP.gap_to_julia(GAP.Globals.HomologyOrigami(GapObj(o))) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All uses of gap_to_julia should be replaced by coercions or type assertions to the desire final type, so that functions are understandable and type stable.
So here, the question would be: what kind of data does HomologyOrigami return, and what would be a good corresponding Julia / Oscar type?
Same for all other calls to gap_to_julia.
This is not a must for a first merge of this PR, but would be nice.
| end | ||
|
|
||
| function all_normal_origamis_by_degree(n::Int) | ||
| return GAP.Globals.AllNormalOrigamisByDegree(n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing this should really return a Vector{Origami} and the result should be wrapped a such?
E.g. this might work?
| return GAP.Globals.AllNormalOrigamisByDegree(n) | |
| return [from_GAP_origami(o) for o in GAP.Globals.AllNormalOrigamisByDegree(n)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normal origamis consist of a finite group
Also, I think the naming of this function is really confusing; I'll talk to @wsinuds about this.
fingolfin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A ton of little comments here and there. I hope you are not overwhelmed by this... a lot of it can be just applied with a few clicks here (don't forget to pull the changes made on GitHub into your local clone afterwards)
experimental/Origami/src/generate.jl
Outdated
|
|
||
| # based on Donald E. Knuth The Art of Computer Programming Algorithm H p.300 | ||
| # implementation from Sage | ||
| function product_gray_code(m::Vector{Int})::Vector{Tuple{Int, Int}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| function product_gray_code(m::Vector{Int})::Vector{Tuple{Int, Int}} | |
| function product_gray_code(m::Vector{Int}) |
experimental/Origami/src/generate.jl
Outdated
| end | ||
|
|
||
| for (i, top_seps) in enumerate(cyl_diagram.top) | ||
| top = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type unstable
experimental/Origami/src/generate.jl
Outdated
| lx[j+widths[i]] = j | ||
| end | ||
| end | ||
| lx = perm([x + 1 for x in lx]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally all calls to perm would take the parent symmetric group as first argument
experimental/Origami/src/generate.jl
Outdated
|
|
||
| if length(current) == n | ||
| if sumSoFar <= max | ||
| push!(combinations, copy(current)) # Add the valid combination to the list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| push!(combinations, copy(current)) # Add the valid combination to the list | |
| push!(combinations, copy(current)) # Add the valid combination to the list |
| if sumSoFar <= max | ||
| push!(combinations, copy(current)) # Add the valid combination to the list | ||
| end | ||
| return combinations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return combinations | |
| return combinations |
|
@Sebas777-gif sorry for flooding you with some many comments (note that GitHub doesn't even show them all by default, you need to click on the "show 43 hidden comments" link...). I think a bunch of them could be just applied with a click from the web interface here. But some probably require a bit of discussion. Perhaps you (or possibly multiple people from your group) would also be interested in meeting up with us for a day or two to work on this. Happy to chat about that. |
|
@fingolfin no problem at all, and thanks for your many contributions and suggestions! It is indeed a bit overwhelming though and I've been procrastinating this work for some time now ... I will pitch your idea of meeting up to @wsinuds and @leo-emmerich tomorrow. |
Co-authored-by: Max Horn <[email protected]>
Co-authored-by: Max Horn <[email protected]>
Co-authored-by: Max Horn <[email protected]>
Co-authored-by: Max Horn <[email protected]>
lgoettgens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more random comments
| return (a.h == b.h) && (a.v == b.v) | ||
| end | ||
|
|
||
| function Base.hash(o::Origami, h::UInt=0x000000000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| function Base.hash(o::Origami, h::UInt=0x000000000) | |
| function Base.hash(o::Origami, h::UInt) |
this should not have a default value, as julia base already provides a one-argument hash method
|
|
||
| function veech_group(O::Origami) | ||
| # TODO use hashtables or not? Implement modular subgroup? | ||
| GAP.Globals.ComputeVeechGroupWithHashTables(GapObj(O)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should have an explicit return. Furthermore, it would probably be good to wrap the return value in an Oscar group type
| length(setdiff(degree_list, moved_points(sigma))) + | ||
| length(setdiff(degree_list, moved_points(sigma * x))) + | ||
| length(setdiff(degree_list, moved_points(sigma * y))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| length(setdiff(degree_list, moved_points(sigma))) + | |
| length(setdiff(degree_list, moved_points(sigma * x))) + | |
| length(setdiff(degree_list, moved_points(sigma * y))) | |
| number_of_fixed_points(sigma) + | |
| number_of_fixed_points(sigma * x) + | |
| number_of_fixed_points(sigma * y) |
| cyl_lists = Vector{Vector{Int}}(gap_obj) | ||
| cyl_tuples = [(l[1], l[2]) for l in cyl_lists] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| cyl_lists = Vector{Vector{Int}}(gap_obj) | |
| cyl_tuples = [(l[1], l[2]) for l in cyl_lists] | |
| cyl_tuples = Vector{Tuple{Int, Int}}(gap_obj) |
this could work as well (I haven't tested it)
| error("Stratum not supported!") | ||
| end | ||
|
|
||
| file_path = joinpath(dirname(@__FILE__), "..", "cylinder_diagrams", file_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| file_path = joinpath(dirname(@__FILE__), "..", "cylinder_diagrams", file_name) | |
| file_path = joinpath(@__DIR__, "..", "cylinder_diagrams", file_name) |
lgoettgens
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading the two GAP packages produces a lot of warnings and errors:
#E component `License' must be bound to a nonempty string containing an SPDX ID
#E Validation of package modulargroup from /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/ModularGroup-2.0.0/\
failed
#E component `License' must be bound to a nonempty string containing an SPDX ID
#E Validation of package origami from /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/ failed
#I DeclareGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/cyclic-torus-covers.gd:67
#I DeclareGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/cyclic-torus-covers.gd:72
#I DeclareGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/cyclic-torus-covers.gd:81
#I DeclareGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/cyclic-torus-covers.gd:100
#I DeclareGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/cyclic-torus-covers.gd:110
#I DeclareGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/cyclic-torus-covers.gd:123
#I DeclareGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/cyclic-torus-covers.gd:136
#I DeclareGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/cyclic-torus-covers.gd:151
#I The package 'ArangoDBInterface' is currently not installed. Without this package, the origami database is not available.
#I InstallGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/origami.gi:346
#I InstallGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/origami.gi:366
#I InstallGlobalFunction: too many arguments in /home/lgoe/.julia/scratchspaces/c863536a-3901-11e9-33e7-d5cd0df7b904/gap_packagedir_v4.14/pkg/Origami-2.0.1/lib/origami.gi:371
#I The package 'ArangoDBInterface' is currently not installed. Without this package, the origami database is not available.
I think most (or even all) of them are already fixed in the packages thanks to @fingolfin, but they require a release of the packages to end up here. This needs to be addressed before this PR can be merged.
|
@lgoettgens we are well aware of these issues. @Sebas777-gif and others from Saarbrücken are going to visit end of the month in Kaiserslautern and we'll try to resolve most / all of this. |
We were working for some time on origami functionalities in our fork of Oscar. @fingolfin recommends this draft pull request, although there is probably still work to be done.